Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

introduce addTransportFilter to NettyChannel #3892

Closed
wants to merge 1 commit into from

Conversation

xiaoshuang-lu
Copy link
Contributor

No description provided.

@xiaoshuang-lu
Copy link
Contributor Author

I will offer another patch to enable this feature of okhttp later. :-)

@xiaoshuang-lu
Copy link
Contributor Author

Hi @dapengzhang0, what about this patch?

@dapengzhang0
Copy link
Member

Thanks for the PR. What is the motivation of adding this feature? In my opinion, giving users the power of filtering transport attributes (which is pretty a grpc internal thing) might make the library fragile.

@xiaoshuang-lu
Copy link
Contributor Author

@dapengzhang0, thanks for your comments. Appears grpc server offers the ability to filter transport attributes, so I port this feature to client. In fact, with transport filter functionality, users can insert their business logic to grpc framework, e.g. connection statistics.

@dapengzhang0
Copy link
Member

cc @zhangkun83 who might be familiar about ServerTransportFilter

@dapengzhang0
Copy link
Member

@xiaoshuang-lu can you elaborate more on usecase of connection statistics, and why will it be preferable at Transport level rather than LoadBalancer.Subchannel level? If it is a useful/reasonable usecase, we will consider adding your API. Thanks.

@xiaoshuang-lu
Copy link
Contributor Author

xiaoshuang-lu commented Jan 11, 2018

TransportFilter is very similar to channelActive/channelInactive of ChannelInboundHandlerAdapter.

SEE ALSO #2132

@zhangkun83
Copy link
Contributor

@xiaoshuang-lu, before we add something we must be sure that it will be useful. Do you have an actual use case?

@xiaoshuang-lu
Copy link
Contributor Author

@zhangkun83, thanks for your comments.

  1. I wanna count connections for clients indeed.
  2. It would be beautiful if both server and client have addTransportFilter. Only server has this interface seems a little bit weird. :-)

@zhangkun83
Copy link
Contributor

I'd like to understand your use case better. Please elaborate whether you want to count currently active connections or connections created so far, and most importantly, what you are going to use this count for.

Client-server API symmetry is a non-goal. In fact, client and server have fundamental differences, so in a lot of cases it doesn't make sense to have the same API. Server needs TransportFilter because server doesn't create transports, and it needs a mechanism to know when transports are created. On the client-side, LoadBalancer initiates the creation of Subchannels, which in turn creates transports -- if you implemented your own LoadBalancer, you could listen to handleSubchannelState() and get an idea of how many transports are created and how many are currently alive.

@xiaoshuang-lu
Copy link
Contributor Author

Hi @zhangkun83,

Please elaborate whether you want to count currently active connections or connections created so far

TransportFilter can handle both scenarios: 1. increase counter in transportReady and decrease counter in transportTerminated, 2. only accumulate in transportReady

what you are going to use this count for

do connection statistics and report data to some metric systems :-)

if you implemented your own LoadBalancer, you could listen to handleSubchannelState() and get an idea of how many transports are created and how many are currently alive.

handleSubchannelState() of LoadBalancer may be an alternative way to count connections. I am afraid that this solution seems unclear, inconvenient, and slightly ugly.

@zhangkun83
Copy link
Contributor

The TranportFilter interface does look cleaner than LoadBalancer for you use case. Being able to alter transport attributes, it's more powerful than you need, though it doesn't sound terrible to me. @ejona86 WDYT?

@ejona86
Copy link
Member

ejona86 commented Jan 22, 2018

ServerTransportFilter is one of those things we did because we thought we required it, and then haven't really used it. Yes, we think we still may use it, but...

I'd rather open something like this up as a stats API. Something like a one-level up version of ServerStreamTracer.Factory, which works on transports instead of streams. On client-side we're using an interceptor today, but we aren't really using it for much. We could probably combine this theoretical transport-level stats API with configuring streams. We could maybe even remove the current ClientStreamTracer CallOption, which would restrict the ability to configure it and would encourage using it for its intended purpose.

(@zhangkun83, I know that last statement sort of conflicts with another statement I've said about relying on it being an interceptor more heavily; we'll have to sort things out.)

Part of my problem is the entire "filter" aspect of ServerTransportFilter remains awkward. While it tries to be generic, filtering has really only been useful for one use-case which we thought forced to begin with: initializing a per-transport object and making it available to each call. In #2132 (comment), @rmichela mentioned it felt weird as well. Maybe we could do some other approach there, like some identifier+WeakHashMap.

A stats API would be "look but don't touch," would fit with the other "be careful; you're on the network thread" stats APIs, and align to what people appear to be using it for: stats.

Although I do feel like I could be convinced another way easily enough.

@rmichela
Copy link
Contributor

The only use I've found for the current ServerTransportFilter API is to attach a unique identity to each server transport session, allowing gRPC services to become "session aware".

@xiaoshuang-lu
Copy link
Contributor Author

@rmichela I leveraged ServerTransportFilter to count connections... $facepalm$

@ejona86
Copy link
Member

ejona86 commented Aug 24, 2018

It seems ServerTransportFilter solves the use-case involved here: connection statistics. It doesn't seem there's a use-case to actually filter the attributes. Closing.

@ejona86 ejona86 closed this Aug 24, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Nov 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants